Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: isolate binding EventEmitter #1937

Merged
merged 6 commits into from
Feb 22, 2021
Merged

fix: isolate binding EventEmitter #1937

merged 6 commits into from
Feb 22, 2021

Conversation

Flarna
Copy link
Member

@Flarna Flarna commented Feb 16, 2021

Which problem is this PR solving?

Binding an EventEmitter a second time via another instances of ContextManager shouldn't have side effects to the first ContextManager.

Short description of the changes

Use an unique symbol per ContextManager instance to isolate them.

@Flarna
fix: isolate binding EventEmitter
1d846bb 
Binding an EventEmitter a second time via another instances of ContextManager
shouldn't have side effects to the first ContextManager.

Use an unique symbol per ContextManager instance to isolate them.
@codecov
Copy link

codecov bot commented Feb 16, 2021 •
edited
Loading

Codecov Report

Merging #1937 (8c80ee1) into main (38d1ee2) will increase coverage by 0.12%.
The diff coverage is 96.55%.

@@            Coverage Diff             @@
##             main    #1937      +/-   ##
==========================================
+ Coverage   92.76%   92.88%   +0.12%     
==========================================
  Files         174      174              
  Lines        6108     6061      -47     
  Branches     1265     1250      -15     
==========================================
- Hits         5666     5630      -36     
+ Misses        442      431      -11
Impacted Files Coverage Δ
...sync-hooks/src/AbstractAsyncHooksContextManager.ts 96.87% <96.42%> (-1.24%) ⬇️
...async-hooks/src/AsyncLocalStorageContextManager.ts 100.00% <100.00%> (ø)
...emetry-core/src/platform/node/RandomIdGenerator.ts 87.50% <0.00%> (-6.25%) ⬇️
...etry-exporter-prometheus/src/PrometheusExporter.ts 90.66% <0.00%> (-1.34%) ⬇️
...ages/opentelemetry-exporter-collector/src/types.ts
...ry-exporter-collector/src/CollectorExporterBase.ts
.../opentelemetry-exporter-collector/src/transform.ts
...lemetry-exporter-collector/src/transformMetrics.ts
...kages/opentelemetry-exporter-collector/src/util.ts
...ntelemetry-web/src/enums/PerformanceTimingNames.ts 100.00% <0.00%> (ø)
... and 5 more

@dyladan dyladan added the enhancement New feature or request label Feb 17, 2021
@Flarna

This comment has been minimized.

Sign in to view
@dyladan dyladan added bug Something isn't working and removed enhancement New feature or request labels Feb 17, 2021
obecny
obecny reviewed Feb 17, 2021
View reviewed changes
Copy link
Member

@obecny obecny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some minor things

packages/opentelemetry-context-async-hooks/src/AbstractAsyncHooksContextManager.ts Show resolved Hide resolved
packages/opentelemetry-context-async-hooks/test/AsyncHooksContextManager.test.ts Outdated Show resolved Hide resolved
packages/opentelemetry-context-async-hooks/src/AbstractAsyncHooksContextManager.ts Outdated Show resolved Hide resolved
packages/opentelemetry-context-async-hooks/src/AbstractAsyncHooksContextManager.ts Outdated Show resolved Hide resolved
obecny
obecny approved these changes Feb 18, 2021
View reviewed changes
Copy link
Member

@obecny obecny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

approving, the last thing is minor anyway leaving this up to you

@Flarna Flarna merged commit 9437d7e into open-telemetry:main Feb 22, 2021
@Flarna Flarna deleted the async-hooks-no-leak branch February 22, 2021 20:43
@dyladan dyladan mentioned this pull request Mar 1, 2021
Merged
@dependabot dependabot bot mentioned this pull request Mar 8, 2021
Merged
This was referenced Mar 8, 2021
Closed
Closed
Closed
Closed
@DTPierreGutierrez DTPierreGutierrez mentioned this pull request May 29, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MSNev MSNev MSNev approved these changes

@vmarchaud vmarchaud vmarchaud approved these changes

@dyladan dyladan dyladan approved these changes

@obecny obecny obecny approved these changes

@johnbley johnbley Awaiting requested review from johnbley

@legendecas legendecas Awaiting requested review from legendecas

@markwolff markwolff Awaiting requested review from markwolff

@mayurkale22 mayurkale22 Awaiting requested review from mayurkale22

@mwear mwear Awaiting requested review from mwear

@naseemkullah naseemkullah Awaiting requested review from naseemkullah

@OlivierAlbertini OlivierAlbertini Awaiting requested review from OlivierAlbertini

Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Flarna @obecny @dyladan @vmarchaud @MSNev